add SofaStream.verify#138
Conversation
There was a problem hiding this comment.
Thanks for the effort. I think this is great to have. I suggested a few small changes and to think about a good way to efficiently and comprehesively test the added functionality.
Oh and please already add this to the HISTORY. We do not have a test to enforce this yet but should already start this new best practice.
|
|
||
| def _verify_open_file(self, issue_handling, mode): | ||
| """Verify currently opened NetCDF file without loading numeric data.""" | ||
| import sofar as sf |
There was a problem hiding this comment.
Imports should be at the top. Any reason that this is here?
| issue_handling : str, optional | ||
| Defines how detected issues are handled. See | ||
| :py:func:`~sofar.Sofa.verify` for details. | ||
| mode : str, optional | ||
| The SOFA standard is more strict for writing data than for reading | ||
| data. See :py:func:`~sofar.Sofa.verify` for details. | ||
|
|
There was a problem hiding this comment.
I would suggest to copy paste the parameter description from Sofa.verify to make this more self contained.
| placeholders for numeric variables. Thus, numeric data are checked for | ||
| type and dimensions without loading the variable contents into memory. | ||
| String variables are loaded because their values and string lengths can | ||
| be relevant for convention verification. |
There was a problem hiding this comment.
Can you add something like 'see :py:func:~sofar.Sofa.verify for more information on the verification process and rules.'?
| _ = file.Wrong_Attribute | ||
|
|
||
|
|
||
| def test_sofastream_verify(temp_sofa_file, tmp_path_factory, monkeypatch): |
There was a problem hiding this comment.
Two suggestions because this test two things at once:
How about having one test to only check that read_sofa is not called (and add a brief docstring to the test even though the existing tests do not follow this)
Since you are calling Sofa.verify, which is already tested in test_sofa_verify.py, we should think about a good way to test only the added functionality here. Would it be sufficient to test this against sf.io.read_sofa? This would mean to test if
- string variables and attributes are loaded as intended
- all variables exist and have the same type and shape
I think this could be done if the 'fake' sofa file that is created in SofaStream._verify_open_file is returned in an intermediate step (so we could test it) and verified in a next step. So maybe split the functionality into
SofaStream._read_open_file_for_verificationandSofaStream._verify_open_file
There was a problem hiding this comment.
I am also thinking that we could also add upgrade_conventions in a similar way. Going through with this fully could imply implementing all Sofa methods. Then, we should probably work with inheritance.
@f-brinkmann should we just concentrate on verify in this PR or zoom out a bit already?
There was a problem hiding this comment.
@artpelling I would definately focus on verify at the moment. I see how for example adding variables can be helpful for example to sequentially write data from measurements into SOFA files, but we should maybe check if its possible and worth the effort.
hoyer-a
left a comment
hoyer-a
left a comment
There was a problem hiding this comment.
Thanks, code looks good and this is a great addition.
I only have a conceptual question, since this would differ from how SofaStream is being used so far.
| with Dataset(self._filename, mode="r") as file: | ||
| self._file = file | ||
| try: | ||
| return self._verify_open_file(issue_handling, mode) | ||
| finally: | ||
| del self._file |
There was a problem hiding this comment.
This would allow using SofaStream without context manager
stream = SofaStream(fp)
stream.verify()
which currently only verify would support. Do we want this?
Also then
if hasattr(self, "_file") and self._file.isopen():
throws a RecursionError unless we add
if name == '_file':
raise AttributeError(name)
to __getattr__
There was a problem hiding this comment.
Using the context manager in my opinion is quite valuable, since it avoids corrupting the file in case it does not get closed etc.
Is there a built-in solution to this with netCDF?
There was a problem hiding this comment.
Not sure if i got your answer right. My question is more about what the user can do with SofaStream.
Currently everything only works within a context, e.g.
with SofaStream(fp) as file:
file.do_something()
data = file.access_some_attribute
What verify now does, is internally open the file in a context manager if _file is not open / doens't exist, so this becomes possible, but only works for verify:
sofastream = SofaStream(fp)
sofastream.verify()
There was a problem hiding this comment.
Ah, thanks. I misinterpreted your comment.
I'd vote for consistency then. But it's not a hard opinion.
There was a problem hiding this comment.
@hoyer-a As far as I remember, the lack of a verify method was one of the reasons leading to the context manager solution not working on the original sofa file reader. Do you think there's an option to combine the two classes now that that is solved?
If so, I'd move my comment to a new issue.
There was a problem hiding this comment.
Given that reading entire sofa files is handled by sf.io.read_sofa and the Sofa-class only stores data / metadata without any io functionality, i don't think it make sense to combine the two classes right now. Sounds like a bigger, more fundamental change.
4 participants
SofaStreamhas noverifymethod. For verification of conventions, one would need to read the entire file into memory which defeats the purpose ofSofaStream. I've added averifymethod to solve that. The method reads everything but the actual numerical data from the (potentially too large for memory).sofainto aSofain-memory object and callsSofa.verify.Changes proposed in this pull request:
SofaStream.verifynp.arraytonp.asarrayinutils.pyto avoid always loading everything into memoryDisclaimer:
I partially used an LLM for the NetCDF4 reading stuff. Local tests seem to work well